New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move celopenapi/model to staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/ #109959
Conversation
Hi @mk46. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cc @jpbetz |
I'm supportive of this change. We've almost entirely rewritten this code at this point making it impossible for us to "rebase" the code from its third party origin. @jiahuif Do you have any concerns with this code move? I don't expect it to impact the work you are doing but wanted to double check. I don't see any particularly obvious problem with the license file changes since we're retaining the same license. But if anyone with expertise on that could weight in, that would be helpful. |
/assign |
Getting warning from go-lint
Should I remove both in order to make go-lint happy? |
Yes. Linter errors are expected as part of this code move since the linters are disabled for third_party directories. Removing unused unexported code is safe. |
@jpbetz Removed unused code. Please allow CI to run tests. |
/triage accepted |
PTAL |
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
…kg/apiserver/schema/cel/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
@TristonianJones are you okay with this change from an attribution perspective? We're changed it a lot, to the point that it wouldn't be practical to rebase. And moving it out of third_party enables linting, which is desirable. |
@jpbetz I'm happy that what I did could serve as an inspiration and give an idea that developed into its own full-fledged offering. Please feel free to change the attribution with my blessing. :) |
Thanks @TristonianJones! /assign @lavalamp This is ready for approval |
Thanks, looks like there was one additional author on a couple commits in that package in https://github.com/google/cel-policy-templates-go/commits/master/policy/model. It would be good to get their ack as well. I'm not sure the best way to capture what is effectively re-contributing the same code to this project under the Kubernetes license. At the very least, I'd like to have something captured in the commit itself and not just a github comment. |
(mechanical change lgtm) |
@jinmmin would you be willing to add a comment here that it's okay to relicense the contributions you've made to https://github.com/google/cel-policy-templates-go/commits/master/policy/model to the Kubernetes project? We've forked a small section of code from cel-policy-templates. We will add some notes in the commit history to try to retain some attribution. Thanks! |
@jpbetz, these commits are part of cel-policy-templates, so if the owner @TristonianJones is fine with the change, I have no objection :). |
Since authors are OK with attribution change (see above comments), this lgtm. /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpbetz, lavalamp, mk46 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…kg/apiserver/schema/cel/ (kubernetes#109959) Co-authored-by: Manish Kumar <manish.kumar1@india.nec.com>
…kg/apiserver/schema/cel/ (kubernetes#109959) Co-authored-by: Manish Kumar <manish.kumar1@india.nec.com>
What type of PR is this?
Move
celopenapi/model
tostaging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/
Add one of the following kinds:
/kind cleanup
Optionally add one or more of the following kinds if applicable:
/area code-organization
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #108346
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: